Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make sure all assertion rules run at least once #1586

Merged
merged 28 commits into from
Nov 15, 2024

Conversation

basiliskus
Copy link
Contributor

@basiliskus basiliskus commented Nov 14, 2024

Description

Validate that all assertion rules in assertion_definitions.json are used at least once int the Automated Test

Issue

#1525

@basiliskus basiliskus changed the base branch from main to story/1523/org-azure-container November 14, 2024 18:17
Copy link

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis ✅

1525 - Fully compliant

Fully compliant requirements:

  • Evaluate whether all rules in assertion_definitions.json have been run in Automated Test
⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Error Handling
Consider adding error handling for the case when rules cannot be loaded or run.

Copy link

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Score
Enhancement
Add exception handling in the runRules method to ensure errors during rule execution are logged

Ensure that the runRules method handles exceptions properly by logging errors when
exceptions occur during rule execution.

rs-e2e/src/main/java/gov/hhs/cdc/trustedintermediary/rse2e/ruleengine/AssertionRuleEngine.java [79-84]

 for (AssertionRule rule : assertionRules) {
-    if (rule.shouldRun(outputHapiMessage)) {
-        rule.runRule(outputHapiMessage, inputHapiMessage);
-        runRules.add(rule);
+    try {
+        if (rule.shouldRun(outputHapiMessage)) {
+            rule.runRule(outputHapiMessage, inputHapiMessage);
+            runRules.add(rule);
+        }
+    } catch (Exception e) {
+        logger.logError("Error running rule: " + rule, e);
     }
 }
Suggestion importance[1-10]: 8

Why: Adding exception handling in the runRules method is crucial to ensure that any errors during rule execution are properly logged, enhancing the robustness and maintainability of the code.

8
Add a method to verify if all rules have been successfully run

Consider adding a method to check if all rules have been successfully run, which can
be useful for debugging and ensuring the completeness of rule execution.

rs-e2e/src/main/java/gov/hhs/cdc/trustedintermediary/rse2e/ruleengine/AssertionRuleEngine.java [67-86]

 public Set<AssertionRule> runRules(Message outputMessage, Message inputMessage) {
     ...
     return runRules;
 }
 
+public boolean allRulesRunSuccessfully() {
+    return assertionRules.equals(runRules);
+}
+
Suggestion importance[1-10]: 6

Why: The suggestion to add a method for checking if all rules have been run successfully could enhance debugging and verification processes, although it's not critical for functionality.

6
Possible bug
Strengthen the assertion to ensure no rules are left unexecuted by checking if toRunRules is empty

Verify that toRunRules.collect { it.name }.isEmpty() is correctly asserting the
expected behavior by ensuring that all rules are indeed run and no rules are left
unexecuted.

rs-e2e/src/test/groovy/gov/hhs/cdc/trustedintermediary/rse2e/AutomatedTest.groovy [67]

-toRunRules.collect { it.name }.isEmpty()
+assert toRunRules.isEmpty() : "There are still rules that haven't been run: " + toRunRules.collect { it.name }
Suggestion importance[1-10]: 7

Why: This suggestion correctly identifies a potential improvement in the test assertion to ensure that no rules are left unexecuted, which is crucial for the accuracy of the test.

7
Best practice
Add exception handling around rule execution to maintain test integrity

Ensure that the engine.runRules(outputMessage, inputMessage) method is called within
a try-catch block to handle potential exceptions and maintain test integrity.

rs-e2e/src/test/groovy/gov/hhs/cdc/trustedintermediary/rse2e/AutomatedTest.groovy [62-63]

-def runRules = engine.runRules(outputMessage, inputMessage)
+def runRules = []
+try {
+    runRules = engine.runRules(outputMessage, inputMessage)
+} catch (Exception e) {
+    fail("Failed to run rules due to an exception: " + e.message)
+}
 toRunRules.removeAll(runRules)
Suggestion importance[1-10]: 7

Why: Adding exception handling around the engine.runRules method call in the test is a best practice that helps maintain test integrity by properly handling potential exceptions.

7

@basiliskus basiliskus marked this pull request as ready for review November 14, 2024 19:51
@basiliskus basiliskus changed the title Make sure all assertion rules run Make sure all assertion rules run at least once Nov 14, 2024
Base automatically changed from story/1523/org-azure-container to main November 15, 2024 18:13
Copy link
Contributor

@somesylvie somesylvie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a small comment on naming clarity and a request for a comment, otherwise looks good

Comment on lines 62 to 63
def runRules = engine.runRules(outputMessage, inputMessage)
toRunRules.removeAll(runRules)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like the naming here might be confusing to come back to in the future since the run in runRules can either be a verb (like 'go run the rules') or an adjective ('run rules are rules that already ran'). Maybe they could be evaluatedRules and rulesToEvaluate, or rulesThatRan and rulesToRun, or some other pair that's slightly more distinct from the verb-based method names?

It might also be worth adding a small comment here, like //Check whether all the rules in the assertions file have been run

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, I'll change the name

Copy link
Contributor

@GilmoreA6 GilmoreA6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! No notes.

@basiliskus basiliskus merged commit 7ba2110 into main Nov 15, 2024
17 checks passed
@basiliskus basiliskus deleted the story/1525/assert-all-assertions-run branch November 15, 2024 20:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants